-
Notifications
You must be signed in to change notification settings - Fork 126
8368467: [lworld] Add new flag generation for jimage to support preview mode #1621
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
8368467: [lworld] Add new flag generation for jimage to support preview mode #1621
Conversation
|
👋 Welcome back david-beaumont! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
Webrevs
|
| return resultResources; | ||
| } | ||
|
|
||
| private static ResourcePool getResourcePool( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pulled out so the variable holding it is effectively final and can be used in a lambda expression.
| .addAttribute(ATTRIBUTE_OFFSET, contentOffset) | ||
| .addAttribute(ATTRIBUTE_COMPRESSED, compressedSize) | ||
| .addAttribute(ATTRIBUTE_UNCOMPRESSED, uncompressedSize); | ||
| .addAttribute(ATTRIBUTE_MODULE, moduleName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This got reformatted correctly to 8 indent because of the new entry.
| assertEquals(PKG_FLAG_IS_PREVIEW_ONLY, emptyRef.flags()); | ||
| } | ||
|
|
||
| @Test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test needs updating. The order of the entries has changed. Sorry for not spotting sooner.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I had fixed this locally and somehow it hadn't got patched into this PR. Updated now and the test compiles and passes.
| // later be merged with a non-empty reference for the same package. | ||
| ModuleReference emptyRef = ModuleReference.forEmptyPackage(modName, isPreviewPath); | ||
|
|
||
| // Work down through empty packages to final resource. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This loop and the section after replaces the logic between L191 and L238 in the original code.
Instead of two maps (which would be 4 if we tried to add preview support that way) with near-duplicated logic, there's now just "walk down the path creating empty package nodes until the last segment is reached, then exit and create the resource node in a non-empty package".
The empty/non-empty/preview nature of these packages is all handled when the module references are merged later, but for now they are independent.
| packageToModules.keySet().removeIf(p -> p.isEmpty() || p.equals("META-INF") || p.startsWith("META-INF.")); | ||
| packageToModules.forEach((pkgName, modRefs) -> { | ||
| // Merge multiple refs for the same module. | ||
| List<ModuleReference> pkgModules = modRefs.stream() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is where module references are merged so each package has the right set of flags according to all the resources visited in the module (empty, non-empty, preview, preview-only...). This replaces the use of the two maps (moduleToPackage and packageToModule) and a bunch of other logic.
| List<ModuleReference> refs = pkgNode.getModuleReferences(); | ||
| ByteBuffer byteBuffer = ByteBuffer.allocate(8 * refs.size()); | ||
| byteBuffer.order(writer.getByteOrder()); | ||
| ModuleReference.write(refs, byteBuffer.asIntBuffer(), writer::addString); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Call into the ModuleReference code added in the previous PR to write the package flags and offsets.
| * <p>While there are 8 combinations of these 3 flags, some will never | ||
| * occur (e.g. {@code HAS_NORMAL_CONTENT + IS_PREVIEW_ONLY}). | ||
| * | ||
| * <p>Package node entries are sorted by name, with the exception that (if |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is out of date (along with the test). It's now sorted "preview first" and then "by name". The reason for the change is that I had made an incorrect assumption that I didn't need to process empty packages from preview resources, but I do (they still appear in /packages/xxx directories).
0c0ad6a to
b192c84
Compare
|
@david-beaumont Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information. |
| // TODO: Uncomment the FLAGS_IS_PACKAGE_ROOT test below. | ||
| // return (getFlags() & FLAGS_IS_PACKAGE_ROOT) != 0 | ||
| return getBase().charAt(1) == 'p' | ||
| return (getFlags() & FLAGS_IS_PACKAGE_ROOT) != 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Introducing a flag bit that is carried around in lots of entries but applies to only one is overkill.
The simple test above for the first letter is cleaner and very localized.
| if (!ENABLE_PREVIEW_MODE) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As commented in the 1618 PR, the extra subclasses caused by the overrides can be replaced by the resolve method switching on ordinal().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great idea, thanks.
| ImageStringsWriter strings, | ||
| long contentOffset, long compressedSize, long uncompressedSize) { | ||
| ImageStringsWriter strings, | ||
| long contentOffset, long compressedSize, long uncompressedSize, int previewFlags) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrap or untab to avoid long lines.
| "/modules/modfoo/com/foo/bar"}) | ||
| public void testModuleDirectories_expected(String name) throws IOException { | ||
| try (ImageReader reader = ImageReader.open(image)) { | ||
| try (ImageReader reader = ImageReader.open(image, PreviewMode.DISABLED)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The image variable might be better documented in upper case to make it easier to see its a pre-computed value.
|
|
|
|
||
| private ByteOrder byteOrder; | ||
| private ImageStringsWriter strings; | ||
| private final ByteOrder byteOrder; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made obvious fields final and removed unused, non-public, code.
…orSize=8 Reviewed-by: dzhang, epeter, rrich
Reviewed-by: tschatzl, dholmes
…odyPublishers.noBody() Reviewed-by: dfuchs, vyazici
Reviewed-by: lkorinth, cstein, jpai, syan, serb, prr
Reviewed-by: cjplummer, pchilanomate
…ent cpu extensions/flags Reviewed-by: fyang, luhenry
…Expressions.java fails with ArithmeticException: / by zero - forgot to respect Expression.info Reviewed-by: kvn, mhaessig
…nd must be greater than origin Reviewed-by: chagedorn, thartmann
…ature part of method patterns Reviewed-by: rcastanedalo, aseoane, thartmann
…sInCustomFileSystem.java Reviewed-by: alanb, syan
Reviewed-by: alanb, liach, bpb
…ct.java timed out Reviewed-by: dfenacci, epeter
Reviewed-by: rriggs, iris, lancea
…nd should be removed Reviewed-by: sspitsyn, amenkov
….properties Reviewed-by: azvegint, kizune
…alid or unrecognized bugid: 50510568367702 Reviewed-by: syan, azvegint, kizune, jdv
…int that intervenes between allocation and stores Reviewed-by: ysr
Reviewed-by: darcy, shade, ihse, iris
|
@david-beaumont Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information. |
| PackageNode pkgNode = new PackageNode(pkg, packages); | ||
| pkgNode.addReference(entry.getKey(), false); | ||
| directAccess.put(pkgNode.getPath(), pkgNode); | ||
| // invalid directory entry marked as not directory (see 8131762). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not 100% sure of the value of retaining stderr logging like this, especially if the code continues after ignoring the input, but it's the same behaviour as before, so I'm leaving it as-is.
| String resourceName = pkgPath.substring(pathEnd + 1); | ||
| // Intermediate packages are marked "empty" (no resources). This might | ||
| // later be merged with a non-empty reference for the same package. | ||
| ModuleReference emptyRef = ModuleReference.forEmptyPackageIn(modName, isPreviewPath); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This convinces me that forEmptyPackageIn is a better name than forEmptyPackage, since the passing of the module name as the first parameter no longer raises eyebrows.
| if (resourceNode == null) { | ||
| ModuleReference resourceRef = ModuleReference.forPackageIn(modName, isPreviewPath); | ||
| packageToModules.computeIfAbsent(fullPkgName, p -> new HashSet<>()).add(resourceRef); | ||
| // Init adds new node to parent (don't add resources to directAccess). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be happy to rename the directAccess field. It's really a holder for directories. The fact it's "direct access" is rather a statement of its form (obvious since it's a map really) and not its function.
Thoughts?
| return path; | ||
| } else { | ||
| return path.substring(0, i); | ||
| // Helper to iterate package names up to, and including, the complete name. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this comment isn't clear enough I can elaborate. Basically lets you iterate indices in "foo.bar.baz" as 3, 7, 11, -1 (including an index at the end of the string before it returns -1). This is just here to make the for-loop cleaner where the real business logic is.
| } | ||
|
|
||
| public String removeRadical(Node node) { | ||
| private String removeRadical(Node node) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this method obviously earns its keep, so would be happy to inline it, perhaps with a string constant. But also happy to leave it.
| // A resource location, remove "/modules" | ||
| String s = tree.toResourceName(current); | ||
| current.loc = outLocations.get(s); | ||
| current.setLocation(outLocations.get(s)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calling this via the method adds a check that nothing is being overwritten.
| () -> ImageLocation.getFlags("/packages/pkgname", p -> true)); | ||
| } | ||
|
|
||
| @Test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The getPackageFlags tests check that module references get merged correctly to create the parent directory flags we expect. While adding individual resources you build up a list of references in each package, one for each resource (directly or indirectly) in that package.
This is a key reason for how the new code can simplify things around the outer "add each resource in turn" loop, by just collecting the references and merging them later to get the right result.
| ModuleReference.forEmptyPackage("modbar", true), | ||
| ModuleReference.forEmptyPackage("modbaz", true)); | ||
| int previewOnlyFlags = ImageLocation.getPackageFlags(refs); | ||
| // Note the asymmetry between this and the getFlags() case. Unlike |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a flag combination you can't see for locations in /modules, because "has-preview-version" there means "is-not-a-preview-resource" and thus cannot imply "preview only". But there is no concept of "is-a-preview-resource" for things in the /packages space.
However, the code using the flags doesn't really care about what combinations exist since the flags are designed to be used independently of each other, each answering a specific question at a certain place in the processing code.
hasPreviewVersionmeans:- Put this entry first so we can skip 99% of packages with no preview content during pre-processing.
isPreviewOnlymeans:- Don't add this to the
/packagesdirectory when not in preview mode. - Put the node for this location in the preview-only map to be added separately during directory completion (in preview mode).
- Don't add this to the
| ImageClassLoader loader = new ImageClassLoader(reader, IMAGE_ENTRIES.keySet()); | ||
|
|
||
| // Preview version of classes either overwrite existing entries or are added to directories. | ||
| assertEquals("Preview: com.foo.HasPreviewVersion", loader.loadAndGetToString("modfoo", "com.foo.HasPreviewVersion")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This "load the class, run its toString() method and test the result" could be encapsulated a bit more if you think it would make the intent of the test clearer. E.g.:
assertClassToString("modfoo", "com.foo.HasPreviewVersion", "Preview: com.foo.HasPreviewVersion", loader);
Or even:
loader.assertNormalVersion("modfoo", "com.foo.NormalFoo");
loader.assertPreviewVersion("modfoo", "com.foo.HasPreviewVersion");
Thoughts?
| "/modules/java.base/java/util/IdentityHashMap$KeyIterator.class", | ||
| "/modules/java.base/java/lang/Shutdown.class", | ||
| "/modules/java.base/java/lang/Shutdown$Lock.class"); | ||
| /// Note: This list is inherently a little fragile and may end up being more |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry about this enormous non-diff. It's because of moving the static init into a holder class because we now want "module" + "path" as two separate things, pre-calculated, for benchmarking the new APIs.
… since jdk-22+8 Reviewed-by: phubner, fparain
Reviewed-by: phubner, fparain
Reviewed-by: dsimms
Reviewed-by: vromero
Reviewed-by: mhaessig, thartmann, chagedorn
Reviewed-by: cjplummer
…DK-8350550 Reviewed-by: coleenp, fparain
* fixing tests after refactoring * Fixing up after dependent PR changes * feedback and remove unused code * new tests for ImageLocation * Restoring lost changes and updating some comments. * add system property guard to preview mode * Remove TODOs now jimage version is bumped * jimage writer changes to support preview mode.
897de83 to
6ee44b1
Compare
Adds support for writing preview related flags into jimage files.
Preview mode is complex. It's not nearly as simple as "does something in /modules/xxx/... have an entry in /modules/xxx/META-INF/preview/...".
Specific issues include:
Supporting preview-only resources without forcing a double lookup on everything.
Changing the set of entries in /packages/xxx directories to account for preview only packages in some modules.
Minimising the work done during image reader initialization to only need to process the small number of preview resources (rather than scanning the whole file to look for them).
The new flags added by this code address these issues, but calculating them correctly with only minor adjustments to the existing code was not feasible, it just became a lot larger and very complex.
To address this, a new type (ModuleReference) is introduced to track and then merge information about packages seen in each module. This allows a much simpler inner loop for processing resource paths when building the node tree, combined with a subsequent merging stage to produce the final package information for each module.
Not that since ModuleReference is needed during jimage reading, that class is already present in the previous PR on which this is based, but it starts to be used to calculate the module flags in this PR.
This PR can also adds the ImageReader unit tests for preview mode, which rely on being able to generate jimage files with preview mode flags in.
Compare and review this against #1619.
Progress
Integration blocker
Issue
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/valhalla.git pull/1621/head:pull/1621$ git checkout pull/1621Update a local copy of the PR:
$ git checkout pull/1621$ git pull https://git.openjdk.org/valhalla.git pull/1621/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 1621View PR using the GUI difftool:
$ git pr show -t 1621Using diff file
Download this PR as a diff file:
https://git.openjdk.org/valhalla/pull/1621.diff
Using Webrev
Link to Webrev Comment